Custom Fallback TOML Config #15617
Open
Custom Fallback TOML Config #15617
+904
−219
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // fallback.toml(defaults dir) <- fallback.toml(env CL_CHAIN_FALLBACK) <- ChainSpecific.toml(env CL_CHAIN_DEFAULTS) | ||
| // | ||
| // the custom fallback gets processed and overrides the default fallback | ||
| if path := env.CustomFallback.Get(); path != "" { |
There was a problem hiding this comment.
can we lock this in with a testscripts test like in https://github.com/smartcontractkit/chainlink/pull/14090/files#diff-ea10429fdb1fbc100e3d8082e39e6d40a12f82b48c1f37ab9158b6c3c0e44d81 ?
There was a problem hiding this comment.
Good idea. I'll add that
AER Report: CI Coreaer_workflow , commit , Detect Changes , Scheduled Run Frequency , Clean Go Tidy & Generate , Flakeguard Root Project / Get Tests To Run , Flakeguard Deployment Project , Core Tests (go_core_tests) , lint , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_race_tests) , Core Tests (go_core_fuzz) , Flakeguard Root Project / Run Tests , Flakeguard Root Project / Report , Flakey Test Detection , SonarQube Scan 1. TestRPCClient_SubscribeFilterLogs failed: [go_core_tests]Source of Error:Why: The test Suggested fix: Ensure the test setup correctly simulates the expected JSON-RPC messages. Verify the mock server sends the correct "eth_subscribe" and "logs" messages and fix any JSON formatting issues in the test or mock server. AER Report: Operator UI CI ran successfully ✅ |
EasterTheBunny
force-pushed
the
etb/toml-custom-fallback
branch
from
1d753ac to
436023c
Compare
jmank88
reviewed
Dec 13, 2024
| id := config.ChainID.String() | ||
|
|
||
| // add ChainID to set of default IDs | ||
| DefaultIDs = append(DefaultIDs, chainID) |
There was a problem hiding this comment.
I think it is important not to separate these global var assignments (defaults and defaultNames too) from their declarations.
There was a problem hiding this comment.
If you want to use helpers, then how about returning values to set from init()?
EasterTheBunny
force-pushed
the
etb/toml-custom-fallback
branch
2 times, most recently
from
a449414 to
f57243d
Compare
This commit provides using an existing env var `CL_CHAIN_DEFAULTS` as a path to a custom `fallback.toml`. This allows plugins to define their own set of fallback options apart from the core node which override the default fallback options.
EasterTheBunny
force-pushed
the
etb/toml-custom-fallback
branch
from
f57243d to
4515f8a
Compare
EasterTheBunny
force-pushed
the
etb/toml-custom-fallback
branch
from
ba58f5c to
2a8f287
Compare
mateusz-sekara
approved these changes
Dec 13, 2024
jmank88
reviewed
Dec 13, 2024
| func readConfig(path string, reader func(name string) ([]byte, error), fallback bool) (*big.Big, Chain, error) { | ||
| bts, err := reader(path) | ||
| if err != nil { | ||
| return nil, Chain{}, fmt.Errorf("%w: %s", errRead, err.Error()) |
There was a problem hiding this comment.
Suggested change
| return nil, Chain{}, fmt.Errorf("%w: %s", errRead, err.Error()) | |
| return nil, Chain{}, fmt.Errorf("%w: %w", errRead, err) |
jmank88
reviewed
Dec 13, 2024
| continue | ||
| } | ||
|
|
||
| log.Fatalf("custom defaults override failure (%s): %s", entry.Name(), err.Error()) |
There was a problem hiding this comment.
Suggested change
| log.Fatalf("custom defaults override failure (%s): %s", entry.Name(), err.Error()) | |
| log.Fatalf("custom defaults override failure (%s): %s", entry.Name(), err) |
jmank88
reviewed
Dec 13, 2024
| id := config.ChainID.String() | ||
| // decode from toml to a chain config | ||
| if err := cconfig.DecodeTOML(bytes.NewReader(bts), &config); err != nil { | ||
| return nil, Chain{}, fmt.Errorf("%w %s: %s", errDecode, path, err.Error()) |
There was a problem hiding this comment.
Suggested change
| return nil, Chain{}, fmt.Errorf("%w %s: %s", errDecode, path, err.Error()) | |
| return nil, Chain{}, fmt.Errorf("%w %s: %w", errDecode, path, err) |
jmank88
reviewed
Dec 13, 2024
Comment on lines
+35
to
+39
| // read all default configs | ||
| initReadDefaults() | ||
|
|
||
| // check for and apply any overrides | ||
| initApplyEVMOverrides() |
There was a problem hiding this comment.
I don't think this indirection is adding value. Please consider setting the global vars from this func, one way or another.
https://github.com/smartcontractkit/chainlink/pull/15617/files#r1883872492
jmank88
reviewed
Dec 13, 2024
Comment on lines
+43
to
+47
| errRead = errors.New("error reading file") | ||
| errDecode = errors.New("error in TOML decoding") | ||
| errMissingChainID = errors.New("missing ChainID") | ||
| errNonNilChainID = errors.New("fallback ChainID must be nil") | ||
| errFallbackConfig = errors.New("fallback config") |
There was a problem hiding this comment.
Are these all single use? I think the indirection makes it harder to read.
There was a problem hiding this comment.
Ah I guess errFallbackConfig is actually checked for
jmank88
requested changes
Dec 13, 2024
Comment on lines
+160
to
+165
| if fallback { | ||
| if config.ChainID != nil { | ||
| return nil, Chain{}, fmt.Errorf("%w: found: %s", errNonNilChainID, config.ChainID) | ||
| } | ||
| customDefaults[id] = config.Chain | ||
|
|
||
| return nil, config.Chain, errFallbackConfig |
There was a problem hiding this comment.
We should not be using a custom error to communicate back to the caller when there is not an error, and it seems unnecessary in this case since they passed it in to us in the first place.
jmank88
reviewed
Dec 13, 2024
| if err != nil { | ||
| log.Fatalf("error opening file (name: %v) in custom defaults override directory: %v", entry.Name(), err) | ||
| if errors.Is(err, errFallbackConfig) { |
There was a problem hiding this comment.
We already know when it is a fallback config, because we just passed the boolean arg to readConfig. Let's use that logic directly instead of hacking errors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit provides using an existing env var
CL_CHAIN_DEFAULTSas a path to a customfallback.toml. This allows plugins to define their own set of fallback options apart from the core node which override the default fallback options.CCIP-4395